Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use ESLint:recommended #1207

Merged
merged 12 commits into from
Oct 22, 2019
Merged

use ESLint:recommended #1207

merged 12 commits into from
Oct 22, 2019

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Oct 9, 2019

No description provided.

@nschonni nschonni changed the title chore: ignore currently failing rules use ESLint:recommended Oct 9, 2019
test/tests/link_link.js Outdated Show resolved Hide resolved
@nschonni nschonni force-pushed the eslint-recommended branch from cfffc0b to 3bf5eec Compare October 9, 2019 20:52
@@ -525,8 +525,8 @@ aria.Grid.prototype.handleSort = function (headerNode) {
var comparator = function (row1, row2) {
var row1Text = row1.children[columnIndex].innerText;
var row2Text = row2.children[columnIndex].innerText;
var row1Value = parseInt(row1Text.replace(/[^0-9\.]+/g, ''));
var row2Value = parseInt(row2Text.replace(/[^0-9\.]+/g, ''));
var row1Value = parseInt(row1Text.replace(/[^0-9.]+/g, ''));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I actually prefer the previous version for readablity, so this could always be disabled for these lines

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I prefer it either, though not so strongly that it seems worth disabling linting for the line

@nschonni
Copy link
Contributor Author

nschonni commented Oct 9, 2019

@spectranaut @mcking65 I can split this up if you want but I just used separate commits for each rule addressed to make it easier to look at those diffs

Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Seems like it caught a fair number of substantive issues too :)

test/.eslintrc.json Outdated Show resolved Hide resolved
@@ -525,8 +525,8 @@ aria.Grid.prototype.handleSort = function (headerNode) {
var comparator = function (row1, row2) {
var row1Text = row1.children[columnIndex].innerText;
var row2Text = row2.children[columnIndex].innerText;
var row1Value = parseInt(row1Text.replace(/[^0-9\.]+/g, ''));
var row2Value = parseInt(row2Text.replace(/[^0-9\.]+/g, ''));
var row1Value = parseInt(row1Text.replace(/[^0-9.]+/g, ''));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I prefer it either, though not so strongly that it seems worth disabling linting for the line

}
flag = true;
break;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😂

@@ -104,7 +104,7 @@ FormatToolbar.prototype.updateDisable = function (copyButton, cutButton, pasteBu
};

FormatToolbar.prototype.selectText = function (start, end, textarea) {
if (typeof(textarea.selectionStart != undefined)) {
if (typeof textarea.selectionStart !== "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮

@@ -35,7 +35,7 @@ var PopupMenuAction = function (domNode, controllerObj) {
msgPrefix = 'PopupMenu constructor argument domNode ';

// Check whether domNode is a DOM element
if (!domNode instanceof Element) {
if (!(domNode instanceof Element)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

welp, that was a thing that existed 😱

@nschonni
Copy link
Contributor Author

nschonni commented Oct 21, 2019

@mcking65 @sh0ji @spectranaut can you give the second person thumbs up and merge? It caught a few bugs already, and the unused/undefined might be hiding more, but they would make the diff harder to parse if included here

Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too!

@@ -51,7 +51,7 @@ const focusOnOrInCell = async function (t, cellElement, focusable) {
};

const findColIndex = function () {
const el = document.activeElement;
let el = document.activeElement;
while (!el.hasAttribute('aria-colindex')) {
el = el.parent;
Copy link
Contributor

@spectranaut spectranaut Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow I'm surprise this never came up. I'll make an issue to fix this never execute code path... #1216

@@ -2,5 +2,10 @@
"extends": "eslint:recommended",
"env": {
"browser": true
},
"rules": {
"no-unused-vars": 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to fix this in the future? is there a follow up task for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fixed going forward, but I didn't spin up an issue for the 3 that are turned off right now

@mcking65 mcking65 merged commit d367ea5 into w3c:master Oct 22, 2019
@nschonni nschonni deleted the eslint-recommended branch October 22, 2019 15:51
michael-n-cooper pushed a commit that referenced this pull request Oct 22, 2019
Infrastructure: Add eslint rules from eslint:recommended config (pull #1207)

Perdiscussion in issue #1180, ad the eslint:recommended shareable config to eslint.
Then fix errors.

* chore: switch to eslint:recommended
* chore: eslint fix with new config
* fix: no-const-assign
* fix: no-empty
* fix: no-useless-escape
* fix: no-duplicate-case
* fix: no-constant-condition
* fix: no-unsafe-negation
* chore: ignore no-cond-assign
* fix: no-redeclare
* Option collides with built-in type
* fix: no-prototype-builtins
* chore: ignore currently failing rules
nschonni pushed a commit to nschonni/aria-practices that referenced this pull request Oct 23, 2019
Infrastructure: Add eslint rules from eslint:recommended config (pull w3c#1207)

Perdiscussion in issue w3c#1180, ad the eslint:recommended shareable config to eslint.
Then fix errors.

* chore: switch to eslint:recommended
* chore: eslint fix with new config
* fix: no-const-assign
* fix: no-empty
* fix: no-useless-escape
* fix: no-duplicate-case
* fix: no-constant-condition
* fix: no-unsafe-negation
* chore: ignore no-cond-assign
* fix: no-redeclare
* Option collides with built-in type
* fix: no-prototype-builtins
* chore: ignore currently failing rules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants